Add list of hosts with ports for setting connection #139

df530 · 2022-01-11T17:21:38Z

It is possible to specify multiple hosts to connect to. It approves fault tolerance of connection because when one host isn't alive client can try to connect to another one.
The connection will be set to the first available host.

Client client(ClientOptions()
              .SetHost({
                  ClientOptions::HostPort("host1.com", 8000),
                  ClientOptions::HostPort("host2.com"), /// port is ClientOptions.port
              }));

When ResetConnection is called, it tries to connect to the first host. If connection failed, then it tries to connect to the next host in list, while list doesn't end up (round-robin approach). If list ends up, ResetConnection throws exception of last host.

CLAassistant · 2022-01-11T17:21:43Z

All committers have signed the CLA.

PolyProgrammist · 2022-01-19T21:38:03Z

Looks good to me. I think it's better to merge commits in one. And providing more information about the reasons for commit (fault tolerance) and about the implementation (round robin) in pull request description would be great

filimonov · 2022-01-20T10:16:13Z

The idea/motivation is clean. Maybe it would be clearer to push that logic a bit deeper?

Here we have iteration through different IPs which can be extracted from the same domain name:

clickhouse-cpp/clickhouse/base/socket.cpp

Lines 87 to 88 in b36c87f

    
           for (auto res = addr.Info(); res != nullptr; res = res->ai_next) { 
        
               SOCKET s(socket(res->ai_family, res->ai_socktype, res->ai_protocol));

Maybe we can generalize that code a bit for multiple hosts/ports cases instead of creating one more retry loop outside?

In that case, connect timeouts should work better.

Enmk

Please add proper unit-tests and fix highlighted issues.

Enmk · 2022-01-24T07:08:50Z

clickhouse/client.h

+    /// List of hostnames with service ports
+    struct HostPort {
+        std::string host;
+        std::optional<unsigned int> port;


Please fix: add #include <optional>
Or enable editing PR by maintainers.

Enmk · 2022-01-24T07:32:02Z

tests/simple/main.cpp

@@ -510,6 +510,67 @@ int main() {
                            .SetCompressionMethod(CompressionMethod::LZ4));
            RunTests(client);
        }
+
+        {


Please add unit tests instead, the sample program is not executed in CI/CD and hence your code is not checked. I suggest you to add a separate test-cases, both positive and negative in client_ut.cpp:

Suggested change

{

TEST(ClientCaseConnect, MultipleEndpoints) {

}

Enmk · 2022-01-24T07:36:58Z

clickhouse/client.h

+    struct HostPort {
+        std::string host;
+        std::optional<unsigned int> port;
+
+        explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) {
+        }
+    };


Please rename HostPort to Endpoint (here and everywhere else), it is a bit more clear. Also, you can significantly simplify the struct definition:

Suggested change

struct HostPort {

std::string host;

std::optional<unsigned int> port;

explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) {

}

};

struct Endpoint {

std::string host;

std::optional<unsigned int> port = std::nullopt;

}

};

Enmk · 2022-01-24T07:39:49Z

clickhouse/client.h

+        explicit HostPort(std::string host, std::optional<unsigned int> port = std::nullopt) : host(std::move(host)), port(std::move(port)) {
+        }
+    };
+    DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{});


Suggested change

DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{});

/** Set endpoints (host+port), only one is used.

* Client tries to connect to those endpoints one by one, on the round-robin basis:

* first default enpoint (set via SetHost() + SetPort()), then each of endpoints, from begin() to end(),

* the first one to establish connection is used for the rest of the session.

* If port part is not specified, default port (@see SetPort()) is used.

*/

DECLARE_FIELD(endpoints, std::vector<Endpoint>, SetEndpoints, {});

Enmk · 2022-01-24T07:48:46Z

clickhouse/client.cpp

@@ -296,73 +320,100 @@ void Client::Impl::Ping() {
 }

 void Client::Impl::ResetConnection() {


⚠ If there was a temporary communication problem, the client could re-connect to a different server (which I believe may be unexpected by the user).

Please make sure that client tries to reconnect to the same server first (options_.send_retries times).

Also I suggest to explicitly allow users to reset connection endpoint, maybe via separate function or ResetConnection() overload/parameter.

E.g.:

ResetConnection() - try reconnecting to current endpoint.

ResetConnectionEndpoint() - try connecting to different endpoint.

Discussed in direct messages that:
ResetConnectionEndpoint() should try to connect to different endpoints one by one only one time. If it doesn't work, throw an exception. It should be called on the client start and it should be called N times. Also the user can call it in order to connect to different endpoint.

ResetConnection() should try to reconnect to the current endpoint N times. It doesn't try to connect to other endpoints. If it doesn't work, throw an exception. So it's behaviour doesn't change

Enmk · 2022-01-24T08:21:19Z

clickhouse/client.cpp

@@ -167,6 +190,7 @@ class Client::Impl {
 #endif

    ServerInfo server_info_;
+    std::optional<ClientOptions::HostPort> connected_host_port_;
 };




Please modify endpoints creation to simplify the ResetConnection()

Suggested change

ClientOptions modifyClientOptions(ClientOptions opts)

{

if (!opts.host.empty())

opts.endpoints.insert(opts.endpoints.begin(), ClientOptions::Endpoint{opts.host, opts.port});

return opts;

}

Client::Impl::Impl(const ClientOptions& opts)

: options_(modifyClientOptions(opts))

{

Enmk · 2022-01-24T08:22:38Z

clickhouse/client.cpp

@@ -296,73 +320,100 @@ void Client::Impl::Ping() {
 }

 void Client::Impl::ResetConnection() {
+    connected_host_port_.reset();
+    for (int i = -1; i < int(options_.hosts_ports.size()); ++i) {
+        const ClientOptions::HostPort& host_port = i == -1 ? ClientOptions::HostPort(options_.host) : options_.hosts_ports[i];


Please update the list of endpoints before copying it to a options_ so you don't have to do this. (see above)

Enmk · 2022-01-24T08:34:01Z

clickhouse/client.h

@@ -196,6 +206,8 @@ class Client {

    const ServerInfo& GetServerInfo() const;

+    const std::optional<ClientOptions::HostPort>& GetConnectedHostPort() const;


Please specify in comments when this function may return nullopt.

Enmk · 2022-01-24T08:39:38Z

tests/simple/main.cpp

+            ClientOptions::HostPort correct_host_port = ClientOptions::HostPort("localhost", 9000);
+            Client client(ClientOptions()
+                              .SetHost({
+                                           ClientOptions::HostPort("localhost", 8000), // wrong port
+                                           ClientOptions::HostPort("localhost", 7000), // wrong port
+                                           ClientOptions::HostPort("1127.91.2.1"), // wrong host
+                                           ClientOptions::HostPort("1127.91.2.2"), // wrong host
+                                           ClientOptions::HostPort("notlocalwronghost"), // wrong host
+                                           ClientOptions::HostPort("another_notlocalwronghost"), // wrong host
+                                           correct_host_port,
+                                           ClientOptions::HostPort("localhost", 9001), // wrong port
+                                           ClientOptions::HostPort("1127.911.2.2"), // wrong host
+                                       })
+                              .SetPingBeforeQuery(true));


BTW, this allows simplifying client code:

Suggested change

ClientOptions::HostPort correct_host_port = ClientOptions::HostPort("localhost", 9000);

Client client(ClientOptions()

.SetHost({

ClientOptions::HostPort("localhost", 8000), // wrong port

ClientOptions::HostPort("localhost", 7000), // wrong port

ClientOptions::HostPort("1127.91.2.1"), // wrong host

ClientOptions::HostPort("1127.91.2.2"), // wrong host

ClientOptions::HostPort("notlocalwronghost"), // wrong host

ClientOptions::HostPort("another_notlocalwronghost"), // wrong host

correct_host_port,

ClientOptions::HostPort("localhost", 9001), // wrong port

ClientOptions::HostPort("1127.911.2.2"), // wrong host

})

.SetPingBeforeQuery(true));

ClientOptions::Endpoint correct_endpoint{"localhost", 9000};

Client client(ClientOptions()

.SetEndpoints({

{"localhost", 8000}, // wrong port

{"localhost", 7000}, // wrong port

{"1127.91.2.1"}, // wrong host

{"1127.91.2.2"}, // wrong host

{"notlocalwronghost"}, // wrong host

{"another_notlocalwronghost"}, // wrong host

{correct_endpoint},

{"localhost", 9001}, // wrong port

{"1127.911.2.2"}, // wrong host

})

.SetPingBeforeQuery(true));

Enmk

Please add proper unit-tests and fix highlighted issues.

Enmk · 2022-05-03T09:06:07Z

@DF5HSE ping?

PolyProgrammist · 2022-05-03T10:11:49Z

@Enmk hi, I have just started to continue working on this pr. Hope it will be ready soon

MikhailBurdukov · 2023-05-15T13:23:10Z

Hi!
We think this feature will be very useful for us, so I would like to complete it. Can we reopen PR or is it better to open a new one?

MikhailBurdukov · 2023-05-23T08:27:21Z

@Enmk ping?

df530 added 9 commits November 22, 2021 18:45

Add setting host list and round robin prototype

0d87218

Add timeout and tests (not debugged)

0b74858

Some changes, not fixed test, but improve action

d713ce7

Fix tests

0f2da33

Delete extra changes

25b7fdc

Fix review comments

662a27d

Divided catch (...) block

53affd2

Add descriptioin of feature to README

fabac4d

Fix font size

edf5421

df530 added 2 commits January 11, 2022 21:04

merge new feature with master

563b7b6

Delete unused operator== overload

13294d3

Enmk suggested changes Jan 24, 2022

View reviewed changes

Enmk reviewed Jan 24, 2022

View reviewed changes

Enmk suggested changes Jan 24, 2022

View reviewed changes

Enmk mentioned this pull request Feb 4, 2022

Add socket injection into client #147

Merged

Enmk closed this Apr 19, 2023

MikhailBurdukov mentioned this pull request May 25, 2023

Multiple endpoints for connection. #310

Merged

-    DECLARE_FIELD(hosts_ports, std::vector<HostPort>, SetHost,{});
+    /** Set endpoints (host+port), only one is used.
+     * Client tries to connect to those endpoints one by one, on the round-robin basis:
+     * first default enpoint (set via SetHost() + SetPort()), then each of endpoints, from begin() to end(),
+     * the first one to establish connection is used for the rest of the session.
+     * If port part is not specified, default port (@see SetPort()) is used.
+     */
+    DECLARE_FIELD(endpoints, std::vector<Endpoint>, SetEndpoints, {});

		@@ -296,73 +320,100 @@ void Client::Impl::Ping() {
		}

		void Client::Impl::ResetConnection() {

+ClientOptions modifyClientOptions(ClientOptions opts)
+{
+    if (!opts.host.empty())
+        opts.endpoints.insert(opts.endpoints.begin(), ClientOptions::Endpoint{opts.host, opts.port});
+    return opts;
+}
+Client::Impl::Impl(const ClientOptions& opts)
+    : options_(modifyClientOptions(opts))
+{

		@@ -196,6 +206,8 @@ class Client {

		const ServerInfo& GetServerInfo() const;

		const std::optional<ClientOptions::HostPort>& GetConnectedHostPort() const;

Add list of hosts with ports for setting connection #139

Add list of hosts with ports for setting connection #139

Uh oh!

Conversation

df530 commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PolyProgrammist commented Jan 19, 2022

Uh oh!

filimonov commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Enmk left a comment

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

PolyProgrammist May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Enmk Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Enmk left a comment

Choose a reason for hiding this comment

Uh oh!

Enmk commented May 3, 2022

Uh oh!

PolyProgrammist commented May 3, 2022

Uh oh!

MikhailBurdukov commented May 15, 2023

Uh oh!

MikhailBurdukov commented May 23, 2023

Uh oh!

Uh oh!

df530 commented Jan 11, 2022 •

edited

Loading

CLAassistant commented Jan 11, 2022 •

edited

Loading

filimonov commented Jan 20, 2022 •

edited

Loading

Enmk Jan 24, 2022 •

edited

Loading

PolyProgrammist May 6, 2022 •

edited

Loading

Enmk Jan 24, 2022 •

edited

Loading